Some optimization steps (plus bug fixes) #3
Conversation
Co-authored-by: Matthias Hörtenhuber <[email protected]>
| changed_files = changed_files + detect_include_files( | ||
| changed_files, include_files | ||
| ) | ||
| changed_files = changed_files + detect_include_files(changed_files, include_files) |
There was a problem hiding this comment.
Ruff seems to accept both, which I don't understand because it goes over 88 characters?
.github/python/find_changed_files.py
Outdated
| # compare two branches | ||
| diff_index = branch1_commit.diff(branch2_commit) | ||
| # Get the diff between two branches | ||
| diff = repo.git.diff(f"{branch1}..{branch2}", name_only=True) |
There was a problem hiding this comment.
I don't think this is synonymous, this gets all changes between the branches rather than the changes introduced by the PR (I think?)
There was a problem hiding this comment.
I did some tests and it should work. I'm not a fan of string formatting for args in a function though.
There was a problem hiding this comment.
also returning strings separated by newlines is 🤮
There was a problem hiding this comment.
okay, can ignore it then, made for me more sense, because it's closer to the cli git (but not very pythony, I agree)
There was a problem hiding this comment.
Shall we just make this an independent action 🤔 Future stuff.
There was a problem hiding this comment.
even past future stuff 😁 nf-core/tools#725
|
My black seems to format things differently to yours? (and Ruff, which should use the same rules). Ruff: |
what's your prettier version? |
Co-authored-by: Adam Talbot <[email protected]>
v3.2.5 |
| if os.path.basename(os.path.dirname(os.path.dirname(test_yml))) not in [ | ||
| "modules", | ||
| "config", | ||
| "subworkflows", | ||
| ]: | ||
| modname = f"{os.path.basename(os.path.dirname(os.path.dirname(test_yml)))}/{os.path.basename(os.path.dirname(test_yml))}" |
There was a problem hiding this comment.
There's got to be a better way here 😆
There was a problem hiding this comment.
I don't think this file should be included in this PR?
| if len(words) == 2 and re.match( | ||
| r"^(workflow|process|function|pipeline|tag)$", words[1] | ||
| ): | ||
| result.append(line) |
There was a problem hiding this comment.
This is wrong, result is a dict.
| nf_test_files = detect_nf_test_files(changed_files) | ||
| lines = process_files(nf_test_files) | ||
| result = convert_nf_test_files_to_test_types(lines) | ||
| result = convert_nf_test_files_to_test_types(lines, args.types) # Get only relevant results (specified by -t) |
There was a problem hiding this comment.
PEP8 says 72 characters max, this is 110.
.pre-commit-config.yaml
Outdated
There was a problem hiding this comment.
I also think this sort of thing should go in a different PR.
There was a problem hiding this comment.
true, just added it, because I saw we started to have quite a bit of python code and wanted to harmonize it with our other python tooling.
There was a problem hiding this comment.
Open a PR to nf-core/modules, so we can test and review in isolation. When I merged this it was broken and didn't even know where to start looking.
|
cherry-picked: faf952e |
|
I've had to revert faf952e because it didn't work See here: https://github.com/nf-core/modules/actions/runs/8613055957/job/23603598762 |
yes, needs a checkout action first. commit incoming |
adamrtalbot
force-pushed
the
new_tag_strategy
branch
from
c351ea1
to
c345358
Compare
No description provided.